docs: add docs related to contributing #171

liammulh · 2022-10-17T20:38:45Z

This PR adds:

CONTRIBUTING.md (the main contributing file that also contains instructions for submitting a PR)
doc/code-principles.md (our "DRY" and "KISS" doc)
doc/code-style.md (our nascent code style stuff that isn't covered by ESLint and Prettier)
doc/git-scenarios-and-what-to-do-about-them.md (a flowchart for figuring out your Git issues)
doc/pull-request-checklist.md (PR checklists for submitters and reviewers)

This PR removes:

doc/submitting-pull-requests.md (added a section in CONTRIBUTING.md)
doc/contributing.md (added content to new CONTRIBUTING.md)

Also add stub for style doc. Related to #69. Related to #99.

Also add pull request checklist. Add content to style guide.

#162

- Move Git/GitHub stuff to its own file. - Move scenarios to their own file. - Add image.

liammulh · 2022-10-17T20:39:30Z

Related to:

gwhitney · 2022-10-18T14:22:25Z

@katestange are you reviewing this since you reviewed the last PR on this topic that (if I understand correctly) gave rise to this one? Or would you like me to? Just let me know.

katestange

This looks great! I had some suggestions, but overall I think it's in excellent shape!

katestange · 2022-10-18T15:08:31Z

doc/git-scenarios-and-what-to-do-about-them.md

+
+Here are the different scenarios you could find yourself in and what to do
+about them.
+


I think maybe we could explain here what the goal is. Why do I want to do anything and not just leave it as is? Is this "how to get to the point of a PR given your current weird git setup"? Maybe an introductory paragraph saying that the goal is to be working on your own fork, on a branch devoted to this pull request, etc. (a bulleted list maybe). Then people can look at it and say, "oh, I see I'm not set up that way, what do I do?" and then start looking through the flowchart.

It might also be helpful to have something like this (but shorter) in the main doc, saying why you might want to click on the flowchart.

Done in 496c64c and 4f38f81.

katestange · 2022-10-18T15:10:14Z

doc/git-scenarios-and-what-to-do-about-them.md

+                -   [Read about basic Git operations](./working-with-git-and-github#basic-git-operations).
+        -   2.B.2: You are working on a different branch.
+            -   2.B.2.A: You have made commits.
+                -   [Read about basic Git operations](./working-with-git-and-github#basic-git-operations).


It seems weird that the remedy here is to read some stuff. Should it say "congrats, you are all set up correctly, here's some useful info" or "read about these git operations because you aren't set up right, but I don't know how to help you"?

Addressed in 6294e3a and 7a08b13.

katestange · 2022-10-18T15:12:02Z

doc/git-scenarios-and-what-to-do-about-them.md

+        -   [Clone your fork](./working-with-git-and-github#clone-a-repo).
+        -   [Create a branch](./working-with-git-and-github#create-a-branch).
+        -   [Push a branch](./working-with-git-and-github#push-a-branch).
+        -   [Read about basic Git operations](./working-with-git-and-github#basic-git-operations).


Everything ends with "read about basic git operations" as if that's the final step. But I would have expected the final step to be something like "Now you are working on a dedicated branch on your own fork, congrats! Read about about basic git operations here (link) and get to work making a cool visualizer!"

Addressed in 7a08b13.

katestange · 2022-10-18T15:14:03Z

doc/git-scenarios-and-what-to-do-about-them.md

+-   1: You have a clone of a Numberscope repository.
+    -   1.A: You haven't made changes.
+        -   [Create a fork](./working-with-git-and-github#create-a-fork).
+        -   [Clone your fork](./working-with-git-and-github#clone-a-repo).


many of these links don't seem to work, did you need a file extension?

Fixed in 7a08b13.

katestange · 2022-10-18T15:16:25Z

doc/pull-request-checklist.md

+        changed files and no warnings/errors.
+-   [ ] The PR builds by running npm run build. (This also checks type
+        correctness.) There should be no errors, and for now the only allowed
+        warning is the one about some assets being too big.


If there's an issue about this assets warning (will it eventually go away?) then we should reference this file there so when that goes away this file is updated.

Created #173.

katestange · 2022-10-18T15:17:16Z

doc/pull-request-checklist.md

+        correctness.) There should be no errors, and for now the only allowed
+        warning is the one about some assets being too big.
+-   [ ] The PR passes all tests. (Right now, just by running npm run
+        test:unit.)


Ditto, what's the mechanism for updating this when there are more tests? Maybe we should put a timestamp on these "right now" and "for now" comments? At least that way, someone who is looking at the file can tell how stale they are.

Added timestamp and created #174.

katestange · 2022-10-18T15:17:54Z

doc/pull-request-checklist.md

+        submitter list, but be sure to exercise as many randomly selected
+        behaviors as you have time for, definitely including but not limited
+        to the ones nominally affected by the PR. This should be done in
+        `npm run preview` mode after a successful build.


thanks, i didn't know that about preview mode

This is Glen's writing, but yeah preview mode allows you to view the docsite as well as interact with frontscope. However, it's built, so no hot module reload.

katestange · 2022-10-18T15:18:25Z

doc/working-with-git-and-github.md

+```sh
+git stash pop
+```
+


in addition to or instead of the git stash apply?

Instead of. Clarified in 8c5ac25.

katestange · 2022-10-18T15:19:09Z

doc/working-with-git-and-github.md

+```
+
+The name of the remote is most likely `origin` if you are pushing to a fork of
+one of the Numberscope repositories.


Add something about how you can view your remote names to double check (you told me that command on slack).

Oh, I see you do something like that below, maybe you should reorganize / link so that info is available here and there somehow.

Added section on checking remotes in 4d7ced0.

katestange · 2022-10-18T15:21:13Z

CONTRIBUTING.md

+   way to do this is to navigate to the page of your fork of
+   numberscope/frontscope and click "Sync fork" (see the picture below).
+   ![A screenshot of the Sync fork option](./doc/img/sync-fork.png) Another
+   way to do this is to


I like the screenshots!

For #171 (comment)

For: - #171 (comment) - #171 (comment)

For #171 (comment)

liammulh · 2022-10-18T20:32:10Z

@katestange, I believe I've addressed all of your feedback. Let me know if there's anything else I overlooked. Thanks!

gwhitney · 2022-10-18T20:35:26Z

I just want to comment that someone (other than Liam) should look at the on-line documentation (as generated by either/both of npm run doc:serve or npm run build followed by npm run preview with all of these changes) and make sure it appears reasonably organized/grouped/navigable. If you've already done that, Kate, great, just let us know. If not and you want me to do it, let me know. Thanks!

katestange · 2022-10-18T21:02:19Z

Hi both, Glen, I did not think to do that. If you want to take a quick look and look over the PR as it stands, that would be great. I think it's looking good.

gwhitney · 2022-10-19T00:47:13Z

OK, I am going to assume that all of the above conversations are resolved. But I am guessing you didn't examine the new collection of docs online, Liam, as the outline no longer corresponds well to the collection of pages available. The outline structure is set in the mkdocs.yml file, in the "nav" block. You will see, for example, that the nav block refers to doc/contributing.md, but there is no longer any such file, leading to a 404 as you navigate the outline. So that at least needs to change. And also, any page in doc/ that is not otherwise mentioned ends up in the "Other Information" category. So if that's not where you want it, you need to either mention it explicitly in nav, or at least have another pattern in nav that will match it (i.e., you could make a subdirectory of the ones you want under the Contributing heading, and then use a glob pattern to grab just those). You may just want to mention each one by its full file name, though, in the nav section, so that they come out in a logical order (if a pattern matches multiple files, I think they are just presented in alphabetical order, for example as with the Visualizers).

Do you want to take a stab at reorganizing the nav section of the mkdocs.yml to present the new collection of files well? This is definitely something that should be done before merging. If you find the format/structure there unclear/difficult to use let me know and we can discuss how to make it more accessible. Thanks!

gwhitney · 2022-10-19T00:48:53Z

Oh, and mkdocs reported the following warning:

Documentation file 'doc/extending.md' contains a link to
            'doc/contributing.md' which is not found in the documentation files.

so it looks like there's also still another dead link that needs fixing before a merge. Thanks for handling.

liammulh · 2022-10-19T02:33:33Z

But I am guessing you didn't examine the new collection of docs online

Yeah, I didn't. I got too focused on making the docs themselves and didn't think of checking the docsite. Thank you for reminding us!

Do you want to take a stab at reorganizing the nav section of the mkdocs.yml to present the new collection of files well?

I would like to do this, but I am having trouble. When I do npm run build, I'm not getting the dist/doc/ directory built, but I'm not seeing errors either. When I run npm run preview, there are no docsite pages. This is strange because npm run build and npm run preview have worked for me in the past. Is there something I'm missing, @gwhitney? I activate the virtual environment before npm run build. I can't think of why dist/doc/ isn't being built.

liammulh · 2022-10-19T02:40:23Z

I blew away my .venv directory and re-ran the postinstall script and that seems to have fixed the issue I was having.

For #171 (comment)

Change titles in some of the docs so they look better in the sidebar. For #171 (comment)

So that the docsite looks good. Part of #171.

For #171 (comment)

liammulh · 2022-10-19T03:15:51Z

Do you want to take a stab at reorganizing the nav section of the mkdocs.yml to present the new collection of files well?

I took a stab at it in 7187a2f and b354337.

Oh, and mkdocs reported the following warning

Addressed in e0bcd94.

I also removed the checkboxes in the PR checklist (6f12825) because they don't render properly on the docsite and I thought it looked weird. Let me know if you think I should put them back since it's a checklist.

@gwhitney, let me know if you like the ordering of the docs in the sidebar.

gwhitney · 2022-10-19T18:31:22Z

Great, no more warnings. (And I am fine either way on the checkboxes.)
Remaining suggestions before merge (and they are suggestions, not firm requests; let me know when you've processed them all, whether or not you decide to take action on any particular ones):

Advertise the "onboarding" doc at the top of "CONTRIBUTING.md", maybe with a second section entitled "If you are new to software development and like written instructions..."
Move "onboarding" up to be the second entry in the Contributing header of the outline, since it's very basic/starting-out-ish.
Somewhere in the getting started/onboarding docs do mention in the material that will be standardly read through that for someone submitting changes, you want to be working on a branch in a fork -- right now the onboarding gets you to a fork but never mentions branching. The idea is to avoid ending up in one of the "Git scenarios"
Shorten the title of "Git scenarios and what to do about them" so that it fits on one line -- maybe "Gitting it right" uf you want to go a little casual, or "Handling Git situations" if not, or any other phrasing you like...
In the Code principles, DRY section, last bullet, I think indexing into an array is another technique that's sometimes very useful to avoid a long switch statement or if / else if / else conditional... In other words, the code isn't always appropriate for a loop to solve the big ugly conditional.
On the Code Style page, first sentence, I'd refer to what prettier and ESLint do as "code formatting" rather than "code style".
On the PR checklist page, in the submitters list, I would add a bullet "Read over the Reviewer checklist and try to make sure in advance that your code is going to proceed as smoothly through the review as possible."
Put "Using package manager scripts" next to "Working with Git and Github". They have a very similar flavor. I'd put the package manager scripts one first of the two, as I'd guess I run npm commands more than twice as often as git commands. They could either both go near the top where the Git stuff is now, since they are kind of basic, or right at the end where the package manager stuff is because they are kind of like reference appendices. Either is fine, whichever makes more sense to you -- I just think they should be adjacent (or even merged) because they have closely related information in fairly similar format.

Thanks for all of your time and effort on this important PR!

for 1 in #171 (comment)

for 2 in #171 (comment)

for 3 in #171 (comment)

for 4 in #171 (comment)

for 5 in #171 (comment)

for 6 in #171 (comment)

for 7 in #171 (comment)

for 8 in #171 (comment)

liammulh · 2022-10-19T19:02:01Z

@gwhitney, I added your suggestions in the most recent 8 commits.

liammulh · 2022-10-19T19:02:39Z

Hang on, need to fix a warning.

for #171

liammulh · 2022-10-19T19:05:00Z

Okay, should be fine now.

gwhitney · 2022-10-19T21:18:02Z

Made a couple tiny edits. Merging now.

liammulh added 8 commits October 17, 2022 12:17

docs: add contributing doc, principles doc

4bd45e5

Also add stub for style doc. Related to #69. Related to #99.

docs: add more work on contributing

31e96fd

Also add pull request checklist. Add content to style guide.

docs: address feedback in linked PR

530b0f9

#162

docs: add content from old contributing doc

65dd277

docs: remove old contributing doc

a59cd05

docs: move some docs to separate files

9b4f869

- Move Git/GitHub stuff to its own file. - Move scenarios to their own file. - Add image.

docs: fix link to contributing in README

13931ed

docs: fix link in onboarding doc, remove old PR file

5107eab

liammulh added 5 commits October 17, 2022 14:44

docs: add red circle to PR image

5900989

docs: remove old code style doc, add new one

2493cc5

docs: fix links in Git issue flowchart doc

e1c8924

docs: fix formatting in Git issues flowchart doc

72ffa5c

docs: remove stray - in flowchart doc

4032495

katestange reviewed Oct 18, 2022

View reviewed changes

liammulh added 4 commits October 18, 2022 13:48

docs: add goal to git scenarios doc

496c64c

For #171 (comment)

docs: add note about Git scenarios to contrib doc

4f38f81

For #171 (comment)

docs: clarify correct scenarios in scenarios doc

6294e3a

For #171 (comment)

docs: add correct setup congrats, fix links

7a08b13

For: - #171 (comment) - #171 (comment)

liammulh mentioned this pull request Oct 18, 2022

Update console warning comment from pull-request-checklist.md when warning goes away #173

Open

docs: add timestamp to PR checklist comments

b95df1f

For #171 (comment)

liammulh mentioned this pull request Oct 18, 2022

Update PR checklist once new tests are added #174

Closed

liammulh added 2 commits October 18, 2022 14:17

docs: clarify git stash commands

8c5ac25

docs: add section on checking remotes

4d7ced0

For #171 (comment)

liammulh added 4 commits October 18, 2022 20:54

docs: add contributing doc to docsite sidebar

7187a2f

For #171 (comment)

docs: move more items into sidebar, change titles

b354337

Change titles in some of the docs so they look better in the sidebar. For #171 (comment)

docs: get rid of checkboxes in PR checklist

6f12825

So that the docsite looks good. Part of #171.

docs: fix link in extending doc

e0bcd94

For #171 (comment)

liammulh added 8 commits October 19, 2022 12:42

docs: advertise onboarding doc in contrib doc

f68dccf

for 1 in #171 (comment)

docs: make onboarding second item in sidebar

54a1ed2

for 2 in #171 (comment)

docs: emphasize correct Git setup in onboarding doc

5fce879

for 3 in #171 (comment)

docs: shorten name of Git scenarios doc

d4fe70d

for 4 in #171 (comment)

docs: add index into array to code principles doc

960b52d

for 5 in #171 (comment)

docs: say code formatting instead of code style

897e6e2

for 6 in #171 (comment)

docs: add bullet point to PR checklist

bdece07

for 7 in #171 (comment)

docs: put working with PM above git/github in sidebar

fe10192

for 8 in #171 (comment)

refactor: put gitting it right doc in doc dir

d61fdff

for #171

gwhitney added 2 commits October 19, 2022 17:11

doc(Code Principles): Make the two subheaders parallel

700ddca

doc(Code Style): remove slightly redundant language

1f70901

gwhitney merged commit d4e7554 into numberscope:main Oct 19, 2022

gwhitney mentioned this pull request Oct 19, 2022

Make Code Style Guide for Numberscope #69

Closed

liammulh deleted the new_contributing branch November 10, 2022 01:57


		Here are the different scenarios you could find yourself in and what to do
		about them.

docs: add docs related to contributing #171

docs: add docs related to contributing #171

Conversation

liammulh commented Oct 17, 2022

liammulh commented Oct 17, 2022 • edited Loading

gwhitney commented Oct 18, 2022

katestange left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liammulh commented Oct 18, 2022

gwhitney commented Oct 18, 2022 • edited Loading

katestange commented Oct 18, 2022

gwhitney commented Oct 19, 2022

gwhitney commented Oct 19, 2022

liammulh commented Oct 19, 2022

liammulh commented Oct 19, 2022

liammulh commented Oct 19, 2022

gwhitney commented Oct 19, 2022

liammulh commented Oct 19, 2022

liammulh commented Oct 19, 2022

liammulh commented Oct 19, 2022

gwhitney commented Oct 19, 2022

liammulh commented Oct 17, 2022 •

edited

Loading

gwhitney commented Oct 18, 2022 •

edited

Loading